Publicstorageprovider rewrite #2646

butonic · 2022-03-17T21:05:18Z

We cannot use the current publicstorageprovider implementation as it changes the fileid when accessing a file via a public link. This causes problems with the WOPI server when users enter and leave the session. see #2635

This PR uses two types of spaces: grant and mountpoint similar to the sharesstorageprovider. The mountpoint space is however only listed when the scope indicates a public link.

The grant space is used to forward requests to the actual storage provider.

Note that the mountpoint has a different resourceid than the grant.

The mountpoint space uses the publicstorageproviderid as the storageid/spaceid and the token as the opaqueid/nodeid.
The grant space uses the storageid/spaceid and opaqueid/nodeid of the shared resource. This causes requests to be routed directly to the original storage provider.

This is now in alignment to how the sharesstorageprovider works.

While relative access using the /public/{token} mountpoint works, a fileid based access, eg via /dav/spaces/{spaceid}!{nodeid} would need an additional publicstorage middleware that reduces the permissions to what is granted by the link. Currently, the authentication impersonates the owner of the shared resource, which would report the wrong permissions.

double check the public scope does prevent escaping the shared tree
~~add public scope / link middleware (must be a middleware, manipulating permissions in ocdav would not affect CS3 api calls, eg by the WCOPI server)~~ tracked in Add publicstorage middleware #2656
fix test config
test ocis update in [full-ci] use publicstorage rewrite owncloud/ocis#3349

glpatcern · 2022-03-18T07:29:37Z

Interesting finding, thanks @butonic - I guess we did not spot it because of our still hybrid deployment, where the WOPI server bypasses Reva to directly talk to the storage.

butonic · 2022-03-18T11:24:43Z

@glpatcern oh no this was brought to my attention by @wkloucek I still need to find out if the WOPI server works with this implementation or if we need to change the fileid of the mount point ... still trying to make the problem as well as the solution more graspable.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

internal/grpc/interceptors/auth/auth.go

pkg/auth/scope/publicshare.go

C0rby

I can trigger a stack trace by doing the following steps:

Create a folder
Upload an image to that folder
Create a public link of the folder
Open the public link
View image in media viewer
Then there is a stack trace like this:

2022-03-21T15:20:24+01:00 ERR unary code=Unknown end="21/Mar/2022:15:20:24 +0100" from=tcp://127.0.0.1:50094 pkg=rgrpc service=storage start="21/Mar/2022:15:20:24 +0100" time_ns=1370727 traceid=00000000000000000000000000000000 uri=/cs3.storage.provider.v1beta1.ProviderAPI/ListStorageSpaces user-agent=grpc-go/1.45.0
goroutine 9188 [running]:
runtime/debug.Stack()
	/usr/local/go/src/runtime/debug/stack.go:24 +0x65
runtime/debug.PrintStack()
	/usr/local/go/src/runtime/debug/stack.go:16 +0x19
github.com/cs3org/reva/v2/internal/grpc/interceptors/recovery.recoveryFunc({0x53c7560, 0xc009d600f0}, {0x3841580, 0x5331740})
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/recovery/recovery.go:48 +0x31
github.com/grpc-ecosystem/go-grpc-middleware/recovery.recoverFrom({0x53c7560, 0xc009d600f0}, {0x3841580, 0x5331740}, 0xc0081d8be8)
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/recovery/interceptors.go:61 +0x36
github.com/grpc-ecosystem/go-grpc-middleware/recovery.UnaryServerInterceptor.func1.1()
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/recovery/interceptors.go:29 +0x7b
panic({0x3841580, 0x5331740})
	/usr/local/go/src/runtime/panic.go:1038 +0x215
github.com/cs3org/reva/v2/pkg/ctx.ContextMustGetUser({0x53c7560, 0xc009d60450})
	/home/corby/work/go/src/github.com/c0rby/reva/pkg/ctx/userctx.go:47 +0x65
github.com/cs3org/reva/v2/internal/grpc/services/gateway.(*svc).CreateHome(0x2, {0x53c7560, 0xc009d60450}, 0x0)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/services/gateway/storageprovider.go:107 +0x46
github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1._GatewayAPI_CreateHome_Handler.func1({0x53c7560, 0xc009d60450}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/cs3org/go-cs3apis@v0.0.0-20220126114148-64c025ccdd19/cs3/gateway/v1beta1/gateway_api.pb.go:3101 +0x78
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.UnaryServerInterceptor.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0xc009d13520, 0xc009d5c210)
	/home/corby/work/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc@v0.29.0/interceptor.go:325 +0x61c
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/auth.NewUnary.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0xc009d13520, 0xc009d13540)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/auth/auth.go:107 +0x25b
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a
github.com/grpc-ecosystem/go-grpc-middleware/recovery.UnaryServerInterceptor.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0xc0077253f8, 0x14f1b97)
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/recovery/interceptors.go:33 +0xc8
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/log.NewUnary.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0xc009d13520, 0xc009d13580)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/log/log.go:39 +0x9a
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/useragent.NewUnary.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0x1, 0xc009d135a0)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/useragent/useragent.go:38 +0xe9
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/token.NewUnary.func1({0x53c7560, 0xc009d60060}, {0x3ca0040, 0xc009d60000}, 0x3df6fc0, 0xc009d135c0)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/token/token.go:44 +0x159
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d60060}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/appctx.NewUnary.func1({0x53c7560, 0xc009d43fb0}, {0x3ca0040, 0xc009d60000}, 0x18, 0xc009d135e0)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/appctx/appctx.go:42 +0x5ab
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d43fb0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1({0x53c7560, 0xc009d43fb0}, {0x3ca0040, 0xc009d60000}, 0xc000d59bd0, 0x399a460)
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:34 +0xbf
github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1._GatewayAPI_CreateHome_Handler({0x3e70700, 0xc0003f1500}, {0x53c7560, 0xc009d43fb0}, 0xc009d15f20, 0xc002698600)
	/home/corby/work/go/pkg/mod/github.com/cs3org/go-cs3apis@v0.0.0-20220126114148-64c025ccdd19/cs3/gateway/v1beta1/gateway_api.pb.go:3103 +0x138
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0015bc000, {0x53f8be0, 0xc000d7e1a0}, 0xc009d58240, 0xc001ba4ab0, 0x6efe440, 0x0)
	/home/corby/work/go/pkg/mod/google.golang.org/grpc@v1.45.0/server.go:1282 +0xccf
google.golang.org/grpc.(*Server).handleStream(0xc0015bc000, {0x53f8be0, 0xc000d7e1a0}, 0xc009d58240, 0x0)
	/home/corby/work/go/pkg/mod/google.golang.org/grpc@v1.45.0/server.go:1619 +0xa2a
google.golang.org/grpc.(*Server).serveStreams.func1.2()
	/home/corby/work/go/pkg/mod/google.golang.org/grpc@v1.45.0/server.go:921 +0x98
created by google.golang.org/grpc.(*Server).serveStreams.func1
	/home/corby/work/go/pkg/mod/google.golang.org/grpc@v1.45.0/server.go:919 +0x294
2022-03-21T15:20:24+01:00 ERR user not found in context pkg=rgrpc service=storage traceid=00000000000000000000000000000000
2022-03-21T15:20:24+01:00 ERR unary code=Internal end="21/Mar/2022:15:20:24 +0100" from=tcp://127.0.0.1:51036 pkg=rgrpc service=storage start="21/Mar/2022:15:20:24 +0100" time_ns=591242 traceid=00000000000000000000000000000000 uri=/cs3.gateway.v1beta1.GatewayAPI/CreateHome user-agent=grpc-go/1.45.0
2022-03-21T15:20:24+01:00 ERR error calling CreateHome error="rpc error: code = Internal desc = user not found in context" service=proxy

It doesn't seem to affect the functionality but it isn't nice.

C0rby · 2022-03-21T14:35:25Z

Also when updating the role of a share, the PROPFIND still returns the old role. Not sure if this is caused by this PR though.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic · 2022-03-22T08:29:28Z

I can trigger a stack trace by doing the following steps:

confirmed. I prevent that now by disallowing CreateHome when in public scope: 20392fb

butonic requested review from a team, labkode, ishank011 and glpatcern as code owners March 17, 2022 21:05

butonic requested a review from micbar March 17, 2022 21:06

butonic marked this pull request as draft March 17, 2022 21:25

butonic marked this pull request as ready for review March 21, 2022 09:20

butonic self-assigned this Mar 21, 2022

butonic added the bug Something isn't working label Mar 21, 2022

butonic mentioned this pull request Mar 21, 2022

[full-ci] use publicstorage rewrite owncloud/ocis#3349

Merged

butonic added 7 commits March 21, 2022 09:34

introduce scope context

84b434c

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add scopes to context

6eaeb63

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

use mrants and mountpoints for public shares

04bae06

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add changelog

dcdf513

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

update test config

151a1fc

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix lint

83adbbd

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

ocdav: replace public mountpoint fileid with grant fileid

c252c1e

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic force-pushed the publicstorageprovider-rewrite branch from f0d8b7b to c252c1e Compare March 21, 2022 09:35

cleanup

fc6e5d0

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic mentioned this pull request Mar 21, 2022

Add publicstorage middleware #2656

Open

butonic requested review from C0rby and wkloucek March 21, 2022 10:55

kobergj reviewed Mar 21, 2022

View reviewed changes

internal/grpc/interceptors/auth/auth.go Outdated Show resolved Hide resolved

kobergj requested changes Mar 21, 2022

View reviewed changes

pkg/auth/scope/publicshare.go Outdated Show resolved Hide resolved

C0rby reviewed Mar 21, 2022

View reviewed changes

butonic added 2 commits March 21, 2022 15:49

cleanup comments

2833e9b

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

prevent nil by disallowing CreateHome in public scope

20392fb

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic requested review from kobergj and C0rby March 22, 2022 08:29

kobergj approved these changes Mar 22, 2022

View reviewed changes

butonic merged commit df406ab into cs3org:edge Mar 23, 2022

butonic deleted the publicstorageprovider-rewrite branch March 23, 2022 08:57

micbar mentioned this pull request Mar 29, 2022

Release 1.19.0 Tech Preview owncloud/ocis#3377

Closed

22 tasks

wkloucek mentioned this pull request May 3, 2022

edge: WOPI server may encounter two different fileids for same resource #2635

Closed

kobergj mentioned this pull request Oct 14, 2022

[tests-only] FixRescanning #3355

Merged

kiranparajuli589 mentioned this pull request Oct 20, 2022

[tests-only][full-ci] bump owncloud core commit for the tests (edge) #3380

Merged

kiranparajuli589 mentioned this pull request Oct 28, 2022

[tests-only][full-ci] Bump the oC/core commit ID for the tests" #3411

Merged

kobergj mentioned this pull request Dec 19, 2022

Fix Tags pkg #3564

Merged

kobergj mentioned this pull request May 22, 2023

Use defer to invalidate ID Cache #3910

Closed

kobergj mentioned this pull request Mar 1, 2024

Extend ServiceAccount Permissions #4544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publicstorageprovider rewrite #2646

Publicstorageprovider rewrite #2646

butonic commented Mar 17, 2022 •

edited

Loading

glpatcern commented Mar 18, 2022

butonic commented Mar 18, 2022

C0rby left a comment

C0rby commented Mar 21, 2022

butonic commented Mar 22, 2022 •

edited

Loading

Publicstorageprovider rewrite #2646

Publicstorageprovider rewrite #2646

Conversation

butonic commented Mar 17, 2022 • edited Loading

glpatcern commented Mar 18, 2022

butonic commented Mar 18, 2022

C0rby left a comment

Choose a reason for hiding this comment

C0rby commented Mar 21, 2022

butonic commented Mar 22, 2022 • edited Loading

butonic commented Mar 17, 2022 •

edited

Loading

butonic commented Mar 22, 2022 •

edited

Loading